Conversation
|
Thanks for the contribution! Please review the labels and make any necessary changes. |
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the signup user flow, transitioning from a single state to a more structured, nested form state managed by react-hook-form. This includes the implementation of distinct signup paths for student, partner, and admin users, along with new types, utility functions, and Zod validation schemas for each step. UI components have been updated and new reusable components like AgreementFooter, CredentialsFields, FileUploadButton, and OfficeAddressPicker have been added. The SearchBar and Select components were also enhanced to support the new requirements. The review identified several areas for improvement: ensuring correct data sourcing for admin addresses, addressing a potential validation loophole in isSignupStepValid due to a default: return true case, replacing magic numbers in flowConfig.ts with named constants, externalizing hardcoded error messages for better localization, removing a console.log statement, and re-evaluating the immediate navigation behavior in the admin organization type selection for improved user experience.
| export function findAddressOption(value: string | null) { | ||
| if (!value) { | ||
| return null; | ||
| } | ||
|
|
||
| return PARTNER_ADDRESS_OPTIONS.find((item) => item.id === value) ?? null; | ||
| } |
There was a problem hiding this comment.
| }, | ||
| addressSearch: { | ||
| visible: isAddressSearchVisible, | ||
| items: PARTNER_ADDRESS_OPTIONS, |
There was a problem hiding this comment.
| default: | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The isSignupStepValid function includes a default: return true; case. This means any new or unhandled signup steps will automatically be considered valid, potentially masking missing validation logic or bugs. It's safer to explicitly define validation for all steps or to throw an error for unhandled cases to ensure comprehensive validation coverage.
| identity: 16.67, | ||
| role: 50, | ||
| postRoleEntry: 66.67, | ||
| postRoleDetails: 83.33, | ||
| finalize: 100, | ||
| } as const; |
There was a problem hiding this comment.
| code: z.ZodIssueCode.custom, | ||
| path: ["collegeId"], | ||
| message: "단과대를 선택해주세요", | ||
| }); |
There was a problem hiding this comment.
The error message "단과대를 선택해주세요" is a hardcoded string. For better maintainability, future localization, and consistency in UI text management, consider moving this message to a constants file or a localization resource.
References
- This comment relates to the management of UI text. To ensure UI text is consistent, correctly worded, and easily localizable as per design specifications, it should be defined in a centralized location like a constants file or localization resource, rather than being hardcoded.
| code: z.ZodIssueCode.custom, | ||
| path: ["departmentId"], | ||
| message: "학과/부를 선택해주세요", | ||
| }); |
There was a problem hiding this comment.
The error message "학과/부를 선택해주세요" is a hardcoded string. For better maintainability, future localization, and consistency in UI text management, consider moving this message to a constants file or a localization resource.
References
- This comment relates to the management of UI text. To ensure UI text is consistent, correctly worded, and easily localizable as per design specifications, it should be defined in a centralized location like a constants file or localization resource, rather than being hardcoded.
| @@ -0,0 +1,4 @@ | |||
| export type AddressSearchItem = { | |||
There was a problem hiding this comment.
따로 types파일로 빼서 정의하는 타입들에 대한 기준이 있으실까요? 어떤 것들을 따로 타입으로 빼면 좋을지 여쭤봅니당
| ); | ||
| }; | ||
| return ( | ||
| <FormProvider {...formMethods}> |
There was a problem hiding this comment.
FormProvider가 자식계층이랑 같은 열에 있어서 감싸고있는지 몰랐네욥 자식이랑 한칸정도 탭되어 있으면 좋을 것 같슴다 biome이 고쳐줬어야 하는 문제같은데 안잡아준게 의외입니다.. :(
| LoginIntroScreen, | ||
| SignupProgressBar, | ||
| SignupStepContent, | ||
| FormProvider, |
There was a problem hiding this comment.
FormProvider를 react-hook-form에서 직접 import하면 좋지 않을까 싶습니다!
프로젝트 내에 만들어져 있는 컴포넌트랑 헷갈리는 부분이 조금 있는 것 같슴다..
There was a problem hiding this comment.
제가 제대로 이해한게 맞을지 모르겠지만 제가 이해한바로는 상태들을 모아서 관리하고, 타겟 컴포넌트로 상태들을 전달하는 과정에서 여러 처리들 + 통합관리를 위해 여러 레이어를 만들어두신 것 같습니다.
구체적으로는 다음과 같은 프로세스를 거치는 것 같더라구요
1단계 useSignupUserFlow - STEP상태값 + 폼 상태 조작 함수 모임
2단계 useSignupFlowPresentation - STEP 상태값 파생계산
3단계 useSignupFlowController - STEP + 폼 상태 조작함수 전달
4단계 SignupUserFlowWidget - props로 onchange 전달
5단계 SignupStepContent - props로 onchange 전달
6단계 각 섹션 컴포넌트 - props로 onchange 받아서 최종 사용
STEP상태값 + 폼 상태 조작 함수를 props를 통해 총 6단계에 걸쳐서 내보내고 있는데, 코드를 읽어보니 값을 추적하기 많이 어렵더라구요 그래서 한번 개선안에 대해 고민해봤습니다!
-
FormProvider + Controller를 사용해보시는 것 좋을 것 같습니당
FormProvider 훅을 부른 뒤에 Controller로 폼을 감싸면 onchange를 직접 연결해주지 않아도 Provider가 자동으로 넣어줘요
이 패턴을 사용하면 useSignupUserFlow에서 생성한 폼 관리 함수를 6단계에 걸쳐 섹션 컴포넌트로 넣어주지 않아도 되서 데이터흐름을 보기 더 쉬울 것 같습니다. useSignupUserFlow 내부도 STEP만 관리하면 되니 더 가독성이 높아지지 않을까 싶네욥 -
useSignupFlowPresentation -> userSignupUserFlow로 통합
presentation레이어가 어떻게보면 step과 관련된 상태값을 조작하는 레이어잖아요?? 그래서 userSignupUserFlow 내부로 옮겨서 상태값 정의 + 조작을 같이 두면 좋지 않을까 싶습니다 (아니면 userSignupUserFlow +useSignupFlowPresentation을 zustand store하나로 관리하는 것도 좋아보여요!) -
결과적으로 데이터를 다음과 같이 관리할 수 있을 것 같아요
useSignupUserFlow -> STEP만 관리
위젯 => FormProvier 관리(폼 데이터 관리)
각 섹션 => 각자 알아서 FormProvier 값을 가져와서 사용
데이터를 관리하기 위해 다양한 시도들을 하셨고 그 과정에서 여러 레이어들이 나오지 않았나 싶어요! 각 레이어들이 어떤 고민에서 나오게 된 것인지 들어보고 싶습니다 :)
고생하셨습니다..!
📝 요약
⚙️ 작업 내용
작업 내용
회원가입 플로우를 사용자/제휴업체/관리자 기준으로 구현하고,
signup-user-flow 구조를 FSD에 맞게 정리했습니다.
주요 변경사항
1. 회원가입 플로우 구현
사용자 회원가입 플로우 구현
제휴업체 회원가입 플로우 구현
관리자 회원가입 플로우 구현
2. signup-user-flow 구조 리팩토링
widgets/signup-user-flow는 화면 조립 역할 중심으로 정리features/signup-user-flow/model에 플로우 상태, 분기, 검증, 진행바 계산 로직 분리features/signup-user-flow/ui에 step별 section 및 공통 UI 블록 분리controller / presentation / actions / overlays구조로 흐름 제어 정리3. RHF / Zod 적용
4. 공통 컴포넌트 추가 및 정리
5. 에셋 및 UI 보강
변경 파일 범위
src/features/signup-user-flow/**src/widgets/signup-user-flow/**src/shared/ui/select/**src/shared/ui/address-search/**src/shared/ui/SearchBar/**src/shared/assets/icons/**🔗 관련 이슈
✅ 체크리스트
💬 리뷰어에게
2026-04-05.10.20.59.mov